feat(fcm): Migrate topic management to FCM v1 API by lahirumaramba · Pull Request #1240 · firebase/firebase-admin-java

lahirumaramba · 2026-09-17T17:31:06Z

Migrates topic subscription and management from the legacy Instance ID (IID) service (iid.googleapis.com) to the FCM v1 REST API (fcm.googleapis.com/v1/projects/{projectId}/registrations/{token}/topicSubscriptions).

gemini-code-assist

Code Review

This pull request migrates topic subscription and unsubscription operations in FirebaseMessaging from the legacy Instance ID API to the FCM v1 API, while retaining the legacy methods as deprecated. The new implementation in FirebaseMessagingClientImpl handles these operations concurrently using CompletableFuture. However, the current design creates and shuts down a new thread pool for every topic management request, which is highly inefficient and can cause thread exhaustion under high load. It is recommended to initialize a single shared ExecutorService in the constructor with core thread timeout enabled, and reuse it across requests to improve performance and simplify the request handling logic.

lahirumaramba · 2026-09-17T17:46:29Z

/gemini review

gemini-code-assist

Code Review

This pull request migrates topic subscription and unsubscription operations to the FCM v1 API, introducing new asynchronous methods in FirebaseMessagingClientImpl using CompletableFuture and deprecating the legacy Instance ID API methods. The feedback recommends using daemon threads in the default executor to prevent JVM shutdown delays in short-lived applications, and wrapping task submissions in a try-catch block to gracefully handle RejectedExecutionException when custom bounded executors are used.

…ment requests

lahirumaramba · 2026-09-21T19:24:38Z

/gemini review

gemini-code-assist

Code Review

This pull request migrates topic management operations (subscribe/unsubscribe) from the legacy Instance ID API to the FCM v1 API. It introduces a configurable FCM host, adds a dedicated executor service for concurrent topic management requests, and updates the internal client implementation and corresponding tests to support these changes. The review identified a potential issue where a trailing slash in a custom FCM host configuration could lead to malformed URLs, and a code suggestion was provided to normalize the host string.

gemini-code-assist · 2026-09-21T19:30:38Z

+    this.projectId = builder.projectId;
+    this.fcmHost = Strings.isNullOrEmpty(builder.fcmHost) ? DEFAULT_FCM_HOST : builder.fcmHost;
+    this.fcmSendUrl = String.format(FCM_URL, this.fcmHost, builder.projectId);


If builder.fcmHost is configured with a trailing slash (e.g., https://custom.host.com/), it can result in URLs with double slashes (e.g., https://custom.host.com//v1/projects/...). This can cause unexpected 404 errors or authentication failures with some strict API gateways or proxies. Consider normalizing fcmHost by stripping any trailing slash.

Suggested change

this.projectId = builder.projectId;

this.fcmHost = Strings.isNullOrEmpty(builder.fcmHost) ? DEFAULT_FCM_HOST : builder.fcmHost;

this.fcmSendUrl = String.format(FCM_URL, this.fcmHost, builder.projectId);

this.projectId = builder.projectId;

String host = Strings.isNullOrEmpty(builder.fcmHost) ? DEFAULT_FCM_HOST : builder.fcmHost;

if (host.endsWith("/")) {

host = host.substring(0, host.length() - 1);

}

this.fcmHost = host;

this.fcmSendUrl = String.format(FCM_URL, this.fcmHost, builder.projectId);

jonathanedey

LGTM, but with some push back on the new thread pool.

jonathanedey · 2026-09-23T16:17:42Z

+    this.executor = builder.executor != null
+        ? builder.executor
+        : createDefaultExecutor(builder.threadFactory);
+    this.threadFactory = builder.threadFactory;
+  }
+
+  private static ExecutorService createDefaultExecutor(ThreadFactory threadFactory) {
+    ThreadFactory baseFactory =
+        threadFactory != null ? threadFactory : Executors.defaultThreadFactory();
+    ThreadFactory factory = new ThreadFactoryBuilder()
+        .setThreadFactory(baseFactory)
+        .setNameFormat("firebase-messaging-topics-%d")
+        .setDaemon(true)
+        .build();
+    ThreadPoolExecutor pool = new ThreadPoolExecutor(
+        100, 100,
+        60L, TimeUnit.SECONDS,
+        new LinkedBlockingQueue<Runnable>(),
+        factory);
+    pool.allowCoreThreadTimeOut(true);
+    return pool;


I'm hesitant on adding a nested thread executor here. The existing implementation already uses an app level thread manager which developers can configure through options. Adding another layer of threads without developer control could have be a negative side effect of more thread overhead than the developer is expecting.

jonathanedey · 2026-09-23T16:49:16Z

+    List<CompletableFuture<TopicResult>> futures = new ArrayList<>();
+    for (int i = 0; i < registrationTokens.size(); i++) {
+      final int index = i;
+      final String token = registrationTokens.get(i);
+      try {
+        futures.add(CompletableFuture.supplyAsync(
+            () -> sendSingleTopicRequest(token, topicName, isSubscribe, index),
+            this.executor));
+      } catch (RejectedExecutionException e) {
+        futures.add(CompletableFuture.completedFuture(
+            TopicResult.error(index, "REJECTED_BY_EXECUTOR")));
+      }
+    }
+
+    CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join();
+
+    int successCount = 0;
+    List<TopicManagementResponse.Error> errors = new ArrayList<>();
+    for (CompletableFuture<TopicResult> future : futures) {
+      TopicResult result = future.join();
+      if (result.isSuccess()) {
+        successCount++;
+      } else {
+        errors.add(new TopicManagementResponse.Error(result.getIndex(), result.getReason()));
+      }
+    }


I think we solved a similar issue without a new thread pool with sendEachAsync. Are there advantages to this method over the sendEachAsync way?

firebase-admin-java/src/main/java/com/google/firebase/messaging/FirebaseMessaging.java

Lines 236 to 247 in db48c53

// Gather all futures and combine into a list

ApiFuture<List<SendResponse>> responsesFuture = ApiFutures.allAsList(list);

// Chain this future to wrap the eventual responses in a BatchResponse without blocking

// the main thread. This uses the current thread to execute, but since the transformation

// function is non-blocking the transformation itself is also non-blocking.

return ApiFutures.transform(

responsesFuture,

(responses) -> {

return new BatchResponseImpl(responses);

},

MoreExecutors.directExecutor());

Thanks for point this here and in the above comment. I will change it to a similar method we used in sendEach

jonathanedey · 2026-09-23T17:01:24Z

+  private String extractReason(FirebaseMessagingException e) {
+    if (e.getMessagingErrorCode() != null) {
+      return e.getMessagingErrorCode().name();
+    }
+    if (e.getHttpResponse() != null && !Strings.isNullOrEmpty(e.getHttpResponse().getContent())) {
+      try {
+        MessagingServiceErrorResponse parsed = jsonFactory.createJsonParser(
+            e.getHttpResponse().getContent())
+            .parseAndClose(MessagingServiceErrorResponse.class);
+        if (parsed.getMessagingErrorCode() != null) {
+          return parsed.getMessagingErrorCode().name();
+        }
+        if (!Strings.isNullOrEmpty(parsed.getStatus())) {
+          return parsed.getStatus();
+        }
+        if (!Strings.isNullOrEmpty(parsed.getErrorMessage())) {
+          return parsed.getErrorMessage();
+        }
+      } catch (Exception ignore) {
+        // Ignore JSON parsing errors
+      }
+    }
+    if (e.getErrorCode() != null && e.getErrorCode() != ErrorCode.UNKNOWN) {
+      return e.getErrorCode().name();
+    }
+    if (e.getHttpResponse() != null) {
+      switch (e.getHttpResponse().getStatusCode()) {
+        case 400:
+          return "INVALID_ARGUMENT";
+        case 401:
+        case 403:
+          return "PERMISSION_DENIED";
+        case 404:
+          return "NOT_FOUND";
+        case 429:
+          return "RESOURCE_EXHAUSTED";
+        case 500:
+          return "INTERNAL";
+        case 503:
+          return "DEADLINE_EXCEEDED";
+        default:
+          return "UNKNOWN_ERROR";
+      }
+    }
+    return "UNKNOWN_ERROR";
+  }


Same as others languages. Not blocking but I don't think we need to reparse the firebase error. We can keep the custom mapping if nessasary for matching the status codes error codes correctly.

yvonnep165 · 2026-09-23T21:20:04Z

+        case 500:
+          return "INTERNAL";
+        case 503:
+          return "DEADLINE_EXCEEDED";


is 503 "Service Unavailable" and 504 "DEADLINE_EXCEEDED"?

…r topic management - Delegate topic subscriptions asynchronously per token using CallableOperation.callAsync(app) in FirebaseMessaging, matching sendEachAsync pattern and respecting developer-configured ThreadManager - Remove internal ExecutorService and ThreadFactory configuration from FirebaseMessagingClientImpl - Simplify extractReason to rely on MessagingErrorHandler populating getMessagingErrorCode() without reparsing raw JSON - Map HTTP 503 to UNAVAILABLE and HTTP 504 to DEADLINE_EXCEEDED in extractReason - Normalize fcmHost during construction by stripping trailing slashes - Update unit tests in FirebaseMessagingTest and FirebaseMessagingClientImplTest

… assertions

jonathanedey

LGTM, Thanks!

lahirumaramba requested a review from a team September 17, 2026 17:31

lahirumaramba force-pushed the lm-fcm-topics branch from fdfe8ea to 51005bd Compare September 17, 2026 17:32

lahirumaramba added the release-note label Sep 17, 2026

gemini-code-assist Bot reviewed Sep 17, 2026

View reviewed changes

Comment thread src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java Outdated

Comment thread src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java Outdated

gemini-code-assist Bot reviewed Sep 17, 2026

View reviewed changes

Comment thread src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java Outdated

Comment thread src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java Outdated

lahirumaramba assigned jonathanedey Sep 17, 2026

OrlandriaH-G approved these changes Sep 21, 2026

View reviewed changes

lahirumaramba added 3 commits September 21, 2026 15:08

feat(messaging): topic subscription changes

0cab00f

refactor(messaging): reuse shared ExecutorService across topic manage…

db87137

…ment requests

refactor(messaging): use daemon threads and handle rejected executions

5511d97

lahirumaramba force-pushed the lm-fcm-topics branch from d9277eb to 5511d97 Compare September 21, 2026 19:13

gemini-code-assist Bot reviewed Sep 21, 2026

View reviewed changes

lahirumaramba requested a review from jonathanedey September 23, 2026 15:35

jonathanedey reviewed Sep 23, 2026

View reviewed changes

yvonnep165 reviewed Sep 23, 2026

View reviewed changes

lahirumaramba added the release:stage Stage a release candidate label Sep 24, 2026

lahirumaramba added 2 commits September 24, 2026 12:20

test(messaging): ensure thread-safe and order-independent topic batch…

77b0a38

… assertions

jonathanedey approved these changes Sep 24, 2026

View reviewed changes

lahirumaramba merged commit 4124d03 into main Sep 24, 2026
17 checks passed

lahirumaramba deleted the lm-fcm-topics branch September 24, 2026 19:59

lahirumaramba mentioned this pull request Sep 24, 2026

[chore] Release 9.11.0 #1245

Merged

	// Gather all futures and combine into a list
	ApiFuture<List<SendResponse>> responsesFuture = ApiFutures.allAsList(list);

	// Chain this future to wrap the eventual responses in a BatchResponse without blocking
	// the main thread. This uses the current thread to execute, but since the transformation
	// function is non-blocking the transformation itself is also non-blocking.
	return ApiFutures.transform(
	responsesFuture,
	(responses) -> {
	return new BatchResponseImpl(responses);
	},
	MoreExecutors.directExecutor());

Conversation

lahirumaramba commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

lahirumaramba commented Sep 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

lahirumaramba commented Sep 21, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanedey left a comment

Choose a reason for hiding this comment

Uh oh!

jonathanedey Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanedey Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

lahirumaramba Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanedey Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

yvonnep165 Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanedey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lahirumaramba commented Sep 17, 2026 •

edited

Loading